Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Geocaches approval refactoring, fixes #1672, affects #48 #2354

Merged
merged 4 commits into from
Jun 22, 2022

Conversation

rapotek
Copy link
Contributor

@rapotek rapotek commented May 24, 2022

The main cause to do geacaches approval refactoring was unification of sending emails using Email object everywhere, but the code was so deprecated, the full refactoring was needed.

Main changes:

  • The whole functionality is moved from a static HTML page to a dynamic (jQuery + jQuery UI + API) one
  • The list of caches waiting for acceptance is retrieved in one big query containing all necessary data instead of a bunch of queries per cache
  • The emails are formatted using subject, headers and footers commonly used in another email messages
  • Additional information is supplied regarding accepted/rejected caches, it can help an OC team member to remind last action when distracted

One additional external JS libraries are used:

  • moment.js, useful for date and time manipulation, included as a chunk with remote source

The client side has been tested successfully on: Mozilla Firefox 100, Google Chrome 101, MS Edge 101 and Opera 86.

Some visual examples:

  • The list of caches waiting for acceptance:
    Before:
    caches_list_old
    After:
    caches_list_new
  • Cache assigned to reviewer:
    Before:
    cache_assigned_old
    After:
    cache_assigned_new
  • Cache accept confirmation:
    Before:
    cache_accept_confirmation_old
    After:
    cache_accept_confirmation_new
  • Cache accepted:
    Before:
    cache_accepted_old
    After:
    cache_accepted_new
  • No caches to accept:
    Before:
    no_caches_old
    After:
    no_caches_new
  • Email sent to a cache owner:
    Before:
    email_accepted_old
    After:
    email_accepted_new

@deg-pl
Copy link
Member

deg-pl commented Jun 7, 2022

I think we can merge this PR. I have only one comment - see above.
@kojoty what do you think about it?
@rapotek thanks for your contribution. This is the next step to better OC PL code

@kojoty
Copy link
Member

kojoty commented Jun 8, 2022

Hi @rapotek

I still don't have much time for OC and I would like not to block anything here.

Anyway: handlebars vs jsRender is a thing which I would like to discuss.

At first I have nothing to jsRender - I have never used it, maybe it's OK but I think we should not use next new template system
If the issue is {{}} in context of translations I think we should not use it at all in php code and use instead. The reason is that if I remember well it needs to parse all the code with eval in php so I think this is not the best way - I don't have a time to check it now but I remember I disliked {{xyz}} translations because of something and wanted to remove them at all.

The rest seems to be OK.

@rapotek
Copy link
Contributor Author

rapotek commented Jun 9, 2022

Hi @rapotek

I still don't have much time for OC and I would like not to block anything here.

Anyway: handlebars vs jsRender is a thing which I would like to discuss.

At first I have nothing to jsRender - I have never used it, maybe it's OK but I think we should not use next new template system If the issue is {{}} in context of translations I think we should not use it at all in php code and use instead. The reason is that if I remember well it needs to parse all the code with eval in php so I think this is not the best way - I don't have a time to check it now but I remember I disliked {{xyz}} translations because of something and wanted to remove them at all.

The rest seems to be OK.

@kojoty I did not know that you are against existing translation marking in templates. IMHO {{label}} looks better in HTML code than <?= tr('label'); ?>, but that is because I've worked with people who did not want to have anything in common with server-side languages like PHP or Java, but were fluent in HTML, CSS, JavaScript etc.
Anyway, if you are against introducing jsRender, I can make a subtemplate containing handlebars templates, include it in main template and get rid of jsRender. Subtemplates are not evaluated in context of translation marks. I tried to avoid unnecessary file multpilication, hence the idea to keep it all in one file with using another js templating library, because I didn't find anywhere in code a solution to keep handlebars code and translation markings in one main template.
Described above update should be ready soon in this PR.

…d to a subtemplate, template names changed to camelCase, viewcache.php in templates changed to ViewCacheController class link
@rapotek
Copy link
Contributor Author

rapotek commented Jun 9, 2022

I have implemented changes described above and updated the PR description.

@kojoty
Copy link
Member

kojoty commented Jun 9, 2022

@rapotek , napiszę po polsku

IMHO {{label}} looks better in HTML code than <?= tr('label'); ?>

ja się zgadzam, że krótsza składania jest lepsza tylko problem w tym jakim kosztem to uzyskujemy - w przypadku OC użycie klamr {} wymaga recznego parsowania kodu każdej strony i podmieniania stringów i tego nie robi zoptymalizowany interpreter php tylko nasz kod tutaj:

function tpl_do_translate($str)

i później musi być eval

eval('?>' . $sCode);

ogólnie ten sposób mi się nie podoba i wydaje mi się "mało optymalny" - to jest narzut nakładany na kazdy widok przez nas wyświetlany.

Uważam, że warto ten twór oc-owy sluzacy do generowania templatów zastąpić czystym php - nie mozna tego zrobic o ile mamy jeszcze stare templaty uzywające {} i czasem {{}} chyba jako zmiennych - przynajmniej nie zupełnie.

...i to jest powód dlaczego nie lubię tych klamr...

Chętnie bym pokminił czy jest jakis prostszy składniowo sposób na wprowadzenie translacji w czystym php i bez eval() ale jakos na razie nie wpadłem na inne rozwiązanie niż <?=tr()?> - może masz jakiś pomysł?

@rapotek
Copy link
Contributor Author

rapotek commented Jun 9, 2022

@rapotek , napiszę po polsku

IMHO {{label}} looks better in HTML code than <?= tr('label'); ?>

ja się zgadzam, że krótsza składania jest lepsza tylko problem w tym jakim kosztem to uzyskujemy - w przypadku OC użycie klamr {} wymaga recznego parsowania kodu każdej strony i podmieniania stringów i tego nie robi zoptymalizowany interpreter php tylko nasz kod tutaj:

function tpl_do_translate($str)

i później musi być eval

eval('?>' . $sCode);

ogólnie ten sposób mi się nie podoba i wydaje mi się "mało optymalny" - to jest narzut nakładany na kazdy widok przez nas wyświetlany.

Uważam, że warto ten twór oc-owy sluzacy do generowania templatów zastąpić czystym php - nie mozna tego zrobic o ile mamy jeszcze stare templaty uzywające {} i czasem {{}} chyba jako zmiennych - przynajmniej nie zupełnie.

...i to jest powód dlaczego nie lubię tych klamr...

Chętnie bym pokminił czy jest jakis prostszy składniowo sposób na wprowadzenie translacji w czystym php i bez eval() ale jakos na razie nie wpadłem na inne rozwiązanie niż <?=tr()?> - może masz jakiś pomysł?

Też oglądałem ten kod i też nie jestem nim zachwycony. Problem w tym, że nawet bardziej profesjonalne systemy szablonowe bazują na parsowaniu i podstawianiu.
To prawda, eval nie jest fajny i przydałoby się tu coś innego, ale tak na szybko wydaje mi się, że zamiast się skupiać na eliminacji eval warto pójść w stronę czegoś w rodzaju "postaci półskompilowanej", tzn. przepuszczać szablon przez translację i keszować wynik dla danego języka jako skrypt php przeznaczony do includowania . Ponowne utworzenie "postaci półskompilowanej" byłoby przy modyfikacji plików językowych lub źródłowego szablonu, co łatwo wykryć. Taki wstępnie przetłumaczony szablon byłby potem gotowy do wielokrotnego wykonania reszty kodu, która w nim jest.
To taki pomysł na szybko.

@deg-pl
Copy link
Member

deg-pl commented Jun 9, 2022

Ja tam jestem za zastosowaniem czegoś, co jest po prostu profesjonalnym silnikiem template'ów. Po co odkrywać Amerykę od nowa, jeśli są gotowe, dobre rozwiązania? Według mnie zdecydowanie najciekawszym jest Twig. W wersji mobile używamy Smarty, ale Smarty jest sporo słabszy.
Silnik template'ów wymusza poniekąd pisanie lepszego jakościowo kodu. Template to template - nie ma tu miejsca na kod php. A w naszym kodzie w template'ach są nawet zapytania SQL...

@deg-pl
Copy link
Member

deg-pl commented Jun 9, 2022

I have implemented changes described above and updated the PR description.

@kojoty can we merge this PR?
Discusson about template system we can continue elsewhere.

@andrixnet
Copy link
Contributor

If there are significant changes to the template system please have them documented before merge. Thank you.

@rapotek
Copy link
Contributor Author

rapotek commented Jun 9, 2022

If there are significant changes to the template system please have them documented before merge. Thank you.

There are no changes to the template system in this PR. Possible changes are only discussed for future.

@kojoty
Copy link
Member

kojoty commented Jun 9, 2022

@kojoty can we merge this PR?

Sure, please go ahead.

@kojoty
Copy link
Member

kojoty commented Jun 9, 2022

Ja tam jestem za zastosowaniem czegoś, co jest po prostu profesjonalnym silnikiem template'ów. Po co odkrywać Amerykę od nowa, jeśli są gotowe, dobre rozwiązania? Według mnie zdecydowanie najciekawszym jest Twig. W wersji mobile używamy Smarty, ale Smarty jest sporo słabszy.

A ja właśnie jestem przeciw. O ile nie robimy cechowania to moim zdaniem te phpowe silniki działające po stronie serwera są bez sensu - to tylko utrudnia dev. - a głupio robić to można zawsze i żadne tech. rozwiązanie tego nie naprawi - w sensie jak ktoś nie kuma gdzie jest miejsce na zapytania SQL to i template tu nie pomoże.

Czemu tak sądzę: taki system templatow nie wnosi nic a jest kosztem bo przetwarzanie tego jest mniej optymalne nic czystego PHP.

Popatrzcie tu: https://stackoverflow.com/questions/9363215/pure-php-html-views-vs-template-engines-views

Ja uważam, że dla nas najlepszy jest czysty PHP + cache APC na wyniki dużych zapytań do db. Zachowanie całych widoków moim zdaniem słabo się sprawdzi gdy mamy bardzo rozbudowane widoki z dużą ilością dynamicznych danych. Templaty po stronie js mają sens bo to już nie obciąża serwera bezpośrednio.

Poza wszystkim zdaje sobie sprawę że to trochę flame war...

@deg-pl
Copy link
Member

deg-pl commented Jun 10, 2022

Hmm... Jeśli dobrze pamiętam, to Ty (@kojoty) wprowadziłeś system template'ów do JS, podczas gdy przecież można używać czystego JS. Sam kiedyś używałem czystego JS do sklejania HTMLa z danymi z API i to był koszmar. Teraz do frontu używam Vue (SFC) i kod jest niesamowicie przejrzysty.

Co do systemu template'ów PHP - ja widzę trzy podstawowe zalety:

  • cache'owanie template'ów. To zupełnie coś innego niż wrzucanie zapytań SQL do APCu
  • wymuszenie poprawności wzorca architektonicznego. System template'ów nie pozwala na wrzucenie kodu PHP, podczas gdy w "obecnym" mamy nawet zapytania do DB. Kiedy zacząłem używać Symfony przeszkadzała mi jego sztywność - brak dostępu do bazy danych w kontrolerze (a nawet w entity). Teraz uważam, że to jedna z największych zalet - jeśli czegoś nie da się łatwo zrobić, prawdopodobnie nie powinieneś tego robić, by nie złamać architektury i logiki. Kod OC jest tak paskudny pod kątem architektury, że mnie za każdym razem szlag trafia gdy trzeba coś zmienić. Pewnie całkiem sporo jest osób, które pomogłyby rozwijać kod, gdyby ten kod jakośkolwiek normalnie wyglądał
  • dostęp do narzędzi jakie daje system. Można napisać swoje, tylko po co, jeśli są gotowe?

Generalnie myślę, że OC wcześniej czy później upadnie. I to właśnie dlatego, że nie znajdzie się nikt, kto zrówna kod z ziemią i nie użyje nowoczesnych narzędzi tworząc kod od zera. Ten kod już umarł, a my go tylko czasem łatamy.

@rapotek
Copy link
Contributor Author

rapotek commented Jun 10, 2022

I created an issue #2356 for further discussion about templating system. Feel free to fill up that issue with above comments. I suggest to discontinue this subject in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants